Gtneg mul divoptimizations #45604

alexcovington · 2020-12-04T19:03:34Z

Extends the JIT to optimize the cases outlined in #44375.

I used the following test case:

using System;
using System.Runtime.CompilerServices;

namespace Scratch
{
    public class Program
    {
        [MethodImpl(MethodImplOptions.NoInlining)]
        static int M5(int a) => -a / 7;  // optimize to  "a / -7"

        [MethodImpl(MethodImplOptions.NoInlining)]
        static int M6(int a) => -(a / 7); // optimize to  "a / -7"

        [MethodImpl(MethodImplOptions.NoInlining)]
        static int M7(int a) => -a * 7;  // optimize to  "a * -7"

        [MethodImpl(MethodImplOptions.NoInlining)]
        static int M8(int a) => -(a * 7); // optimize to  "a * -7"

        public static void Main(string[] args) {
            Console.WriteLine("{0} {1} {2} {3}", M5(10), M6(10), M7(10), M8(10));
        }
    }
}

jit-diff output:

PS C:\Users\acovingt\source\repos\Scratch> jit-diff diff --base C:\Users\acovingt\source\repos\runtime-master\artifacts\bin\coreclr\windows.x64.Debug\ --diff C:\Users\acovingt\source\repos\runtime\artifacts\bin\coreclr\windows.x64.Debug\ --core_root C:\Users\acovingt\source\repos\runtime-master\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\ --assembly .\bin\Debug\netcoreapp5.0\Scratch.dll --output C:\Users\acovingt\Documents\diffs\
Beginning Crossgen CodeSize Diffs for .\bin\Debug\netcoreapp5.0\Scratch.dll
- Finished 1/1 Base 1/1 Diff [2.9 sec]
Completed Crossgen CodeSize Diffs for .\bin\Debug\netcoreapp5.0\Scratch.dll in 2.94s
Diffs (if any) can be viewed by comparing: C:\Users\acovingt\Documents\diffs\dasmset_16\base C:\Users\acovingt\Documents\diffs\dasmset_16\diff

git diff --no-index --diff-filter=M --exit-code --numstat C:\Users\acovingt\Documents\diffs\dasmset_16\diff C:\Users\acovingt\Documents\diffs\dasmset_16\base

Analyzing CodeSize diffs...
Found 1 files with textual diffs.

Crossgen CodeSize Diffs for .\bin\Debug\netcoreapp5.0\Scratch.dll for  default jit

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 431
Total bytes of diff: 421
Total bytes of delta: -10 (-2.32% of base)
    diff is an improvement.

Top file improvements (bytes):
         -10 : Scratch.dasm (-2.32% of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
          -4 (-23.53% of base) : Scratch.dasm - Scratch.Program:M7(int):int
          -2 (-9.09% of base) : Scratch.dasm - Scratch.Program:M5(int):int
          -2 (-9.09% of base) : Scratch.dasm - Scratch.Program:M6(int):int
          -2 (-13.33% of base) : Scratch.dasm - Scratch.Program:M8(int):int

Top method improvements (percentages):
          -4 (-23.53% of base) : Scratch.dasm - Scratch.Program:M7(int):int
          -2 (-13.33% of base) : Scratch.dasm - Scratch.Program:M8(int):int
          -2 (-9.09% of base) : Scratch.dasm - Scratch.Program:M5(int):int
          -2 (-9.09% of base) : Scratch.dasm - Scratch.Program:M6(int):int

4 total methods with Code Size differences (4 improved, 0 regressed), 2 unchanged.
Completed analysis in 0.65s

EgorBo · 2020-12-05T09:53:14Z

I think you need to perform this optimization only if tree->gtOverflow is false, e.g.:

checked
{
    int x = -y * int.MinValue;
}

Also, I mentioned #44266 in the issue - you need to make sure the constant on the right is not ->IsIconHandle()

EgorBo · 2020-12-05T10:01:14Z

However, even unchecked context won't save you from this case:

class Program
{
    static async Task Main()
    {
        int x1 = -GetValue() / 1;  // OK
        int x2 =  GetValue() / -1; // throws OverflowException even in the unchecked context
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int GetValue() => int.MinValue;
}

So I'd give up if op2 is 1, %type%.MinValue (and, perhaps -1 and 0) + tree->gtOverflow is true

EgorBo · 2020-12-05T10:06:09Z

src/coreclr/src/jit/morph.cpp

+        case GT_MUL:  
+            // -a * C => a * -C, where C is constant
+            // MUL(NEG(a), C) => MUL(a, NEG(C))
+            if (op1->OperIs(GT_NEG) && !op1->IsCnsIntOrI() && op2->IsCnsIntOrI())


!op1->IsCnsIntOrI() - I think you meant op1->gtOp1()->IsCnsIntOrI() here.

EgorBo · 2020-12-05T10:13:34Z

src/coreclr/src/jit/morph.cpp

+                // op1: a
+                // op2: NEG
+                // op2Child: C
+                op1 = op1->AsOp()->gtOp1; // a


I'd rewrite to be:

tree->AsOp()->gtOp1 = op1->gtGetOp1(); DEBUG_DESTROY_NODE(op1); op2->AsIntCon()->SetIconValue(-op2->AsIntCon()->IconValue()); return tree;

ViktorHofer · 2020-12-08T14:50:16Z

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

alexcovington · 2020-12-08T16:50:26Z

@EgorBo Thanks for the feedback. I have these changes made on my dev machine, but the machine is having some issues at the moment. I expect to have these updated by tomorrow. Sorry for the delay!

…-mul-divoptimizations

sandreenko · 2020-12-28T21:35:58Z

Hi @alexcovington, thanks for your PR and sorry for the long review.
Could you please resolve the merge conflicts and run jit-format on your PR when you have time and ping @dotnet/jit-contrib after that.

alexcovington · 2020-12-29T03:09:56Z

Pinging @dotnet/jit-contrib

kunalspathak · 2021-01-05T20:24:17Z

@alexcovington - Looks like there are multiple failures when crossgening System.Private.Corelib.dll. Did you get chance to take a look?

Generating native image of System.Private.CoreLib for windows.x64.Checked. Logging to F:\workspace\_work\1\s\artifacts\log\CrossgenCoreLib_windows__x64__Checked.log
  F:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\crossgen.exe /nologo  /Platform_Assemblies_Paths "F:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\IL" /out "F:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\System.Private.CoreLib.dll" "F:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\IL\System.Private.CoreLib.dll"
  Extra flags on tree [000003]: -C---------
                 [000003] --C---------              *  CAST      int <- long
                 [000014] ---------U--              \--*  CAST      long <- ulong <- byref
                 [000000] ------------                 \--*  LCL_VAR   byref  V00 arg0         
  
  Assert failure(PID 4516 [0x000011a4], Thread: 7996 [0x1f3c]): Assertion failed '!"Extra flags on tree"' in 'System.SpanHelpers:UnalignedCountVector128(byref):long' during 'Morph - Global' (IL size 21)
  
      File: F:\workspace\_work\1\s\src\coreclr\jit\flowgraph.cpp Line: 22230
      Image: F:\workspace\_work\1\s\artifacts\bin\coreclr\windows.x64.Checked\crossgen.exe

alexcovington · 2021-01-05T20:33:47Z

@kunalspathak Sorry, I started looking at this, but it fell of my radar for a little bit.

I'm a little stuck on the Extra flags on tree error, not exactly sure where the flags are added/how to remove them. Any advice would be greatly appreciated!

kunalspathak · 2021-01-06T00:31:55Z

src/coreclr/jit/morph.cpp

+                tree->AsOp()->gtOp1 = op1->gtGetOp1();
+                DEBUG_DESTROY_NODE(op1);
+                op2->AsIntCon()->SetIconValue(-op2->AsIntCon()->IconValue()); // -C
+                return tree;


I think this is the cause of the problem. You return the tree immediately but if you see after the switch-case, we update the tree flags. In your case, when we hit the assert, because GTF_CALL is not yet removed from the node because here, we return immediately. I believe same applies to the GT_MUL case.

I am not too familiar with the morph and probably @sandreenko should confirm.

kunalspathak · 2021-01-06T06:16:24Z

Changes LGTM. Is it possible to get the libraries pmi code size diff?

sandreenko · 2021-01-06T06:41:42Z

src/coreclr/jit/morph.cpp

-*   A GenTree* which points to the new tree. If the tree is not for simd intrinsic,
-*   return nullptr.
-*/
+ *  If a read operation tries to access simd struct field, then transform the


did jit-format cause these changes or were these unintentional edits? Could you please try to delete them and run jit-format again?

sandreenko · 2021-01-06T06:45:08Z

src/coreclr/jit/morph.cpp

@@ -11909,6 +11909,22 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
            return fgMorphCast(tree);

        case GT_MUL:
+            // -a * C => a * -C, where C is constant
+            // MUL(NEG(a), C) => MUL(a, NEG(C))
+            if (fgGlobalMorph && op1->OperIs(GT_NEG) && !op1->gtGetOp1()->IsCnsIntOrI() && op2->IsCnsIntOrI() &&


opts.OptimizationEnabled() check is missed here and in the other cases, when this flag is set to false we don't want to do any CQ transformations, only what is needed for correctness, it is used, for example, in minopts.

sandreenko · 2021-01-06T06:47:11Z

src/coreclr/jit/morph.cpp

+            // -a * C => a * -C, where C is constant
+            // MUL(NEG(a), C) => MUL(a, NEG(C))
+            if (fgGlobalMorph && op1->OperIs(GT_NEG) && !op1->gtGetOp1()->IsCnsIntOrI() && op2->IsCnsIntOrI() &&
+                !op2->IsIconHandle() && op2->AsIntCon()->IconValue() != 1 && op2->AsIntCon()->IconValue() != 0 &&


Why do you check fgGlobalMorph here? Did you want to use !gtIsActiveCSE_Candidate ? gtIsActiveCSE_Candidate means it is not CSE phase (it could be a global morph phase instead) or this tree is not the current candidate that could be dangerous to remove.

I checked fgGlobalMorph because it was checked in #43921, which is what I based most of this work on. I can use !gtIsActiveCSE_Candidate if that would be better.

Still learning about the CoreCLR JIT, I greatly appreciate the advice :).

On second thought I think fgGlobalMorph check is fine here because you are changing node value without updating its VN.
An alternative could be to do the optimization with !gtIsActiveCSE_Candidate but create a new tree for the constant:

tree->AsOp()->gtOp2 = gtNewIconNode(-op2->AsIntCon()->IconValue(), op2->TypeGet()); DEBUG_DESTROY_NODE(op2);

it would allow the optimization to happen in other phases, so I think it would be better.

sandreenko · 2021-01-06T23:54:52Z

src/coreclr/jit/morph.cpp

+            // -a * C => a * -C, where C is constant
+            // MUL(NEG(a), C) => MUL(a, NEG(C))
+            if (fgGlobalMorph && op1->OperIs(GT_NEG) && !op1->gtGetOp1()->IsCnsIntOrI() && op2->IsCnsIntOrI() &&
+                !op2->IsIconHandle() && op2->AsIntCon()->IconValue() != 1 && op2->AsIntCon()->IconValue() != 0 &&


On second thought I think fgGlobalMorph check is fine here because you are changing node value without updating its VN.
An alternative could be to do the optimization with !gtIsActiveCSE_Candidate but create a new tree for the constant:

tree->AsOp()->gtOp2 = gtNewIconNode(-op2->AsIntCon()->IconValue(), op2->TypeGet()); DEBUG_DESTROY_NODE(op2);

it would allow the optimization to happen in other phases, so I think it would be better.

sandreenko · 2021-01-06T23:55:54Z

src/coreclr/jit/morph.cpp

+                !op2->IsIconHandle() && op2->AsIntCon()->IconValue() != 1 && op2->AsIntCon()->IconValue() != 0 &&
+                op2->AsIntCon()->IconValue() != -1 && !tree->gtOverflow())
+            {
+                // tree: MUL


is this the comment for the tree before the transformation?
then it should be

// tree: MUL // op1: Neg // op1Child: a // op2: Const

sandreenko · 2021-01-06T23:56:19Z

src/coreclr/jit/morph.cpp

+            {
+                // tree: DIV
+                // op1: a
+                // op2: NEG


the same here.

src/coreclr/jit/morph.cpp

Co-authored-by: Sergey Andreenko <seandree@microsoft.com>

sandreenko · 2021-01-08T09:57:30Z

Looks good, @alexcovington, do you mind if I push a small change to your branch for this PR? The checks for bad cases that @EgorBo pointed out should be a little bit different. Also, I think it is worth a new test (probably we should place it to https://github.com/dotnet/runtime/tree/master/src/tests/JIT/opt/InstructionCombining) that I expect will behave differently on arm/arm64 so I don't want to bother you with it (for your first PR 😄).

alexcovington · 2021-01-08T16:25:23Z

@sandreenko Absolutely! Just let me know if you need anything else from me 😄.

sandreenko · 2021-01-11T21:40:43Z

1 improvement in SPC:

Top method improvements (bytes):
          -2 (-5.71% of base) : System.Private.CoreLib.dasm - SpanHelpers:UnalignedCountVector128(byref):long (2 methods)

expected improvements on the repro test:

Top method regressions (bytes):
           1 ( 5.56% of base) : NegMulOrDivToConst.dasm - Program:Div2_3(int):long

Top method improvements (bytes):
          -6 (-23.08% of base) : NegMulOrDivToConst.dasm - Program:Div1_4(long):long
          -4 (-50.00% of base) : NegMulOrDivToConst.dasm - Program:Mul1_1(int):int
          -4 (-40.00% of base) : NegMulOrDivToConst.dasm - Program:Mul1_3(int):int
          -4 (-17.39% of base) : NegMulOrDivToConst.dasm - Program:Div1_1(int):int
          -3 (-8.82% of base) : NegMulOrDivToConst.dasm - Program:Div2_2(long):long
          -3 (-13.04% of base) : NegMulOrDivToConst.dasm - Program:Div2_4(long):long
          -2 (-22.22% of base) : NegMulOrDivToConst.dasm - Program:Mul1_5(int):int
          -2 (-33.33% of base) : NegMulOrDivToConst.dasm - Program:Mul2_1(int):int
          -2 (-9.52% of base) : NegMulOrDivToConst.dasm - Program:Div2_1(int):int

the regression is from -(a / int.MinValue); that could be optimized by lower better,

runtime/src/coreclr/jit/lower.cpp

Lines 5323 to 5325 in 323e7a3

    
           // If the divisor is the minimum representable integer value then we can use a compare, 
        
           // the result is 1 iff the dividend equals divisor. 
        
           divMod->SetOper(GT_EQ);

but it is not worth checking for it in my opinion.

Also, I decided to use !optValnumCSE_phase instead of !gtIsActiveCSE_Candidate(tree) because too many trees were changing their values during these transformations and checking them all was too messy.

alexcovington · 2021-01-11T21:46:00Z

@sandreenko Thanks for the help, this is very informative as well! I'll keep it in mind for future PRs.

kunalspathak · 2021-01-11T22:53:48Z

src/tests/JIT/opt/InstructionCombining/NegMulOrDivToConst.cs

+        static int CheckMulNeg()
+        {
+            bool fail = false;
+            if (Mul1_1(3) != -7 * 3)


minor nit: can we make the method names Mul_1 and other more meaningful like Mul_Minus7, etc.?

Thanks @sandreenko ! Should be good to go.

sandreenko · 2021-01-12T02:30:32Z

Thanks @alexcovington for the PR!

alexcovington added 3 commits December 3, 2020 13:50

GT_NEG optimization for multiplication and division

e487fe3

Distribute negation over parenthetical multiplication or division.

7f04f55

Removing duplicate logic that I had put in accidently.

bbaf21f

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 4, 2020

EgorBo reviewed Dec 5, 2020

View reviewed changes

Check overflow and other conditions before performing morph

bc91dcc

alexcovington added 3 commits December 9, 2020 10:17

Merging change that removes src/coreclr/src directory

084a285

Merge branch 'master' of https://github.com/dotnet/runtime into gtneg…

b575ab6

…-mul-divoptimizations

Resolved merge conflict and cleanup morph.cpp

9727f8e

sandreenko self-requested a review December 28, 2020 21:33

alexcovington added 2 commits December 28, 2020 14:09

Resolved merge conflict

7829712

Formatting morph.cpp

b4bf7e9

sandreenko removed their request for review January 5, 2021 00:19

kunalspathak reviewed Jan 6, 2021

View reviewed changes

alexcovington added 2 commits January 5, 2021 20:08

Returning tree after performing smpop again to fix flags

bd9a464

Formatting

9136999

sandreenko reviewed Jan 6, 2021

View reviewed changes

Added check for optimizations, formatting.

2e72f94

JulieLeeMSFT requested a review from sandreenko January 7, 2021 22:26

Using gtIsActiveCSE_Candidate instead of fgGlobalMorph

6467520

sandreenko reviewed Jan 7, 2021

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

alexcovington and others added 2 commits January 7, 2021 15:12

Update src/coreclr/jit/morph.cpp

4a7f9b5

Co-authored-by: Sergey Andreenko <seandree@microsoft.com>

Formatting

cb71a90

Sergey added 3 commits January 11, 2021 12:04

delete formatting changes.

fea92de

Add a test.

8d2dc8e

Change the conditions a bit.

46b8fff

kunalspathak reviewed Jan 11, 2021

View reviewed changes

Better names for the tests.

55d54e6

sandreenko approved these changes Jan 12, 2021

View reviewed changes

sandreenko merged commit f9cf601 into dotnet:master Jan 12, 2021

alexcovington mentioned this pull request Jan 27, 2021

JIT: Optimize simple MUL/DIV arithmetic with GT_NEG #44375

Closed

ghost locked as resolved and limited conversation to collaborators Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gtneg mul divoptimizations #45604

Gtneg mul divoptimizations #45604

alexcovington commented Dec 4, 2020

EgorBo commented Dec 5, 2020

EgorBo commented Dec 5, 2020 •

edited

Loading

EgorBo Dec 5, 2020

EgorBo Dec 5, 2020

ViktorHofer commented Dec 8, 2020

alexcovington commented Dec 8, 2020

sandreenko commented Dec 28, 2020

alexcovington commented Dec 29, 2020

kunalspathak commented Jan 5, 2021

alexcovington commented Jan 5, 2021

kunalspathak Jan 6, 2021

kunalspathak commented Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko Jan 6, 2021

alexcovington Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko Jan 6, 2021

sandreenko commented Jan 8, 2021

alexcovington commented Jan 8, 2021

sandreenko commented Jan 11, 2021

alexcovington commented Jan 11, 2021

kunalspathak Jan 11, 2021 •

edited

Loading

kunalspathak Jan 12, 2021

sandreenko commented Jan 12, 2021

Gtneg mul divoptimizations #45604

Gtneg mul divoptimizations #45604

Conversation

alexcovington commented Dec 4, 2020

EgorBo commented Dec 5, 2020

EgorBo commented Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Dec 8, 2020

alexcovington commented Dec 8, 2020

sandreenko commented Dec 28, 2020

alexcovington commented Dec 29, 2020

kunalspathak commented Jan 5, 2021

alexcovington commented Jan 5, 2021

Choose a reason for hiding this comment

kunalspathak commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Jan 8, 2021

alexcovington commented Jan 8, 2021

sandreenko commented Jan 11, 2021

alexcovington commented Jan 11, 2021

kunalspathak Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Jan 12, 2021

EgorBo commented Dec 5, 2020 •

edited

Loading

kunalspathak Jan 11, 2021 •

edited

Loading